Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate VPA CRD v1 from types.go #3606

Merged
merged 1 commit into from
Dec 7, 2020

Conversation

Ladicle
Copy link
Contributor

@Ladicle Ladicle commented Oct 12, 2020

ref: #3508
In this PR, I generate CRD v1 from types.go using [email protected].

Here are some things to note:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 12, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2020
@bskiba bskiba self-assigned this Oct 16, 2020
Copy link
Member

@bskiba bskiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks interesting.
My main question is, what will happen for an existing vpa deployment that used the old yaml? Is the generated yaml compatible, so that the existing vpa objects will continue working?

fi

# The following commands always returns an error because controller-gen does not accept keys other than strings.
${CONTROLLER_GEN} ${CRD_OPTS} paths="${APIS_PATH}/..." output:crd:dir=${WORKSPACE} ||:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way we can check for this specific error? What will happen if the generator fails for a different error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, controller-gen does not support feature to check for specific error or ignore only the specific error, but we can find other errors in the output. If the generator fails for a different error, this script will also output other than map keys must be strings.

$ hack/generate-crd-yaml.sh
/Users/ladicle/src/k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1/types.go:360:20: map keys must be strings, not int
/Users/ladicle/src/k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1beta2/types.go:340:20: map keys must be strings, not int
Error: not all generators ran successfully
run `controller-gen crd:trivialVersions=false,allowDangerousTypes=true paths=/Users/ladicle/src/k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/... output:crd:dir=/var/folders/p_/hzm_gclx7vs5cdbfbzpl0s4w0000gn/T/tmp.Cytp8OtyCn -w` to see all available markers, or `controller-gen crd:trivialVersions=false,allowDangerousTypes=true paths=/Users/ladicle/src/k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/... output:crd:dir=/var/folders/p_/hzm_gclx7vs5cdbfbzpl0s4w0000gn/T/tmp.Cytp8OtyCn -h` for usage

Another way is to grep the controller-gen output and display a warning message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a not-overly complicated way to indicate that something other went wrong, I would really like to see it. Otherwise it might get really confusing for someone encountering issues.

@@ -3,4 +3,4 @@ resources:
- recommender-deployment.yaml
- updater-deployment.yaml
- vpa-rbac.yaml
- vpa-v1-crd.yaml
- vpa-v1-crd-v1.yaml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe vpa-v1-crd-gen.yaml

@Ladicle
Copy link
Contributor Author

Ladicle commented Oct 19, 2020

what will happen for an existing vpa deployment that used the old yaml? Is the generated yaml compatible, so that the existing vpa objects will continue working?

Yes, there is no change in the yaml field, so it's compatible. If you applied to a cluster which has old vpa, .spec.preserveUnknownFileds will be true, so the behaviour also will not change. Also, I've confirmed that vpa objects continue to work. However, clusters below v1.16 do not support CRD v1, so the new yaml can not be applied.

When the first time to install vpa, .spec.preserveUnknownFields will be false, so if you apply an invalid object to the cluster, you will get an error message.

like this:

$ kubectl apply -f examples/hamster.yaml
error: error validating "examples/hamster.yaml": error validating data: ValidationError(VerticalPodAutoscaler.spec.resourcePolicy.containerPolicies[0]): unknown field "controlledResources" in io.k8s.autoscaling.v1beta2.VerticalPodAutoscaler.spec.resourcePolicy.containerPolicies; if you choose to ignore these errors, turn validation off with --validate=false

Copy link
Member

@bskiba bskiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, left a couple more minor comments.

@@ -39,8 +39,17 @@ if [ $# -gt 2 ]; then
exit 1
fi

SEMVER_RE='v[^0-9]*\([0-9]*\)[.]\([0-9]*\)[.]\([0-9]*\)\([0-9A-Za-z-]*\)'
K8S_VERSION=$(kubectl version --short=true|grep Server|awk '{print $3}')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to check the k8s version - the 0.9 VPA requires 1.16 anyway: https://github.com/kubernetes/autoscaler/tree/master/vertical-pod-autoscaler#compatibility

COMPONENTS="vpa-v1-crd ${COMPONENTS}"
else
COMPONENTS="vpa-v1-crd-v1-gen ${COMPONENTS}"
fi
case ${ACTION} in
delete|diff|print) COMPONENTS+=" vpa-beta2-crd" ;;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant is that we need to add vpa-v1-crd here to delete it on vpa-down.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove version-specific processing and specify only vpa-v1-crd-v1-gen; although the versions of the CRD are different, they define the same VPA resources, we cand delete CR using either crd-v1-crd or crd-v1-crd-gen files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sence, thank you

fi

# The following commands always returns an error because controller-gen does not accept keys other than strings.
${CONTROLLER_GEN} ${CRD_OPTS} paths="${APIS_PATH}/..." output:crd:dir=${WORKSPACE} ||:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a not-overly complicated way to indicate that something other went wrong, I would really like to see it. Otherwise it might get really confusing for someone encountering issues.

@tamalsaha
Copy link
Member

tamalsaha commented Nov 3, 2020

viacheckpoint have int map key that is not supported in JSON, so disable validation only for that field. Along with this, generation script ignores error returned by controller-gen. (ref: #3574)

I don't think this is the right approach, since we are preserving a mistake. I recommend that @kubernetes/sig-api-machinery-api-reviews is consulted for this one. Thanks.

I think one way this can addressed is that introduce a new version of this api that has the correct data types. Then use CRD conversion webhook to migrate to that version.

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Nov 3, 2020
@fejta-bot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@Ladicle
Copy link
Contributor Author

Ladicle commented Nov 4, 2020

I agree with @tamalsaha that we should finally modify the VPA APIs to use string as the map key. Disabling validation of the BucketWeights field is just a workaround.
However, updating both the CRD API version and the VPA API version at once requires a lot of changes. IMHO, updating the CRD version and modifying VPA field will have less impact on the user if we work on them separately.

@bskiba
Copy link
Member

bskiba commented Nov 24, 2020

I agree with @Ladicle on not mixing too many changes at once. I think there is already a lot of benefit from moving to the controller-gen here and the issue with incorrectly defined field is a separate one, which needs to be approached with a lot of care to not break existing users.

Copy link
Member

@bskiba bskiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good.
Small naming change and we should be good to merge.

@@ -3,4 +3,4 @@ resources:
- recommender-deployment.yaml
- updater-deployment.yaml
- vpa-rbac.yaml
- vpa-v1-crd.yaml
- vpa-v1-crd-v1-gen.yaml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vpa-v1-crd-gen

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recheck all file names and unified to vpa-v1-crd-gen. Also, recreated the CRD manifest.

@@ -73,7 +73,7 @@ for i in ${COMPONENTS}; do
ALL_ARCHITECTURES=amd64 make --directory ${SCRIPT_ROOT}/pkg/${i} release
done

kubectl create -f ${SCRIPT_ROOT}/deploy/vpa-v1-crd.yaml
kubectl create -f ${SCRIPT_ROOT}/deploy/vpa-v1-crd-v1-gen.yaml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vpa-v1-crd-gen

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 24, 2020
@bskiba
Copy link
Member

bskiba commented Dec 2, 2020

Looks good, thanks a lot!

Can you squash the commits? Then it's ready to go :)

@Ladicle
Copy link
Contributor Author

Ladicle commented Dec 3, 2020

Thank you for reviewing :) I've squashed all commits.

Copy link
Contributor

@mwielgus mwielgus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 7, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bskiba, Ladicle, mwielgus

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 5781b68 into kubernetes:master Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/vertical-pod-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants